Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Work around missing subprocess members on Google App Engine #1825

Merged
merged 8 commits into from Mar 20, 2013

Conversation

mgiuca-google
Copy link
Contributor

On Google App Engine, the subprocess module exists, but is empty. This patch introduces a replacement module, subprocess_fixed, which provides a stub for subprocess.Popen which is compatible with existing usage.

I noticed that cbook already has a bit of a hack to work around the fact that subprocess.check_output is missing in Python 2.6, so I've moved that into my subprocess_fixed module as well, so now it has two reasons to exist.

Calls to subprocess_fixed.Popen on Google App Engine always raise OSError, which most of the codebase is already prepared for (since that's what happens when the application is missing). I've added a few missing catches of OSError.

Partial fix for Issue #1823.

a2 = Popen('ps -p %d -o osz' % pid, shell=True,
stdout=PIPE).stdout.readlines()
except OSError:
raise NotImplementedError(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm tempted to suggest a RuntimeError here. None-the-less, this is an API change which should be documented in docs/api/api_changes.rst

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Note that I am having trouble building the docs locally, so I'm hoping my ReST syntax is correct!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I agree RuntimeError is more appropriate (since NotImplementedError implies that it is planning to be fixed one day), but I changed this to be compatible with the Windows implementation. Happy to change it if you want. For what it's worth, NotImplementedError is a subclass of RuntimeError.

@pelson
Copy link
Member

pelson commented Mar 15, 2013

Again, this is looking good @mgiuca-google - thanks for your hard work.

@mgiuca-google
Copy link
Contributor Author

Hmm ... looks like Travis failed on Python 3 with this error:

$ python ../matplotlib/tests.py
Traceback (most recent call last):
  File "../matplotlib/tests.py", line 8, in <module>
    import matplotlib
  File "/home/travis/virtualenv/python3.2/lib/python3.2/site-packages/matplotlib-1.3.x-py3.2-linux-x86_64.egg/matplotlib/__init__.py", line 136, in <module>
    from matplotlib.compat import subprocess
  File "/home/travis/virtualenv/python3.2/lib/python3.2/site-packages/matplotlib-1.3.x-py3.2-linux-x86_64.egg/matplotlib/compat/subprocess.py", line 18, in <module>
    from . import subprocess
ImportError: cannot import name subprocess

The command "python ../matplotlib/tests.py" exited with 1.

I'm not sure why my code import subprocess was magically changed to from . import subprocess -- can you think of what is doing this and a way around it? In any case, I think the problem is that the name subprocess is now overloaded. From within the compat package, it could refer to either my overriding version, or the official subprocess library. This doesn't sound like a very good idea, but then if we can't call it "subprocess" then we're back to requiring all imports of this module to use "as".

I think this can be worked around using __import__ magic. Does that sound like a good idea, or would you rather just rename the module?

@pelson
Copy link
Member

pelson commented Mar 19, 2013

I'm not sure why my code import subprocess was magically changed to from . import subprocess -- can you think of what is doing this and a way around it

Python 2's default is to use relative imports before absolute ones, so import subprocess in python 2 is the same as from . import subprocess in python 3 (assuming you have a submodule called subprocess).

In order to get the behaviour you desire, I think you can just add the line from __future__ import absolute_import into your subprocess compatibility module.

It does look a little like 2to3 is being a little over zealous here...

…ubprocess.

cbook: Moved check_output to subprocess_fixed.
backend_pgf: Import check_output from subprocess_fixed instead of cbook.
If subprocess.Popen is missing (for example, on Google App Engine), replaces it
with a dummy version that raises OSError. In these environments, calls to
subprocess will be handled properly as if the called app could not be found,
instead of raising AttributeError.
- The subtle change to cbook.report_memory's exception types.
- Moving subprocess_fixed to compat.subprocess.
This prevents the module from importing itself (as opposed to the global
subprocess module) on Python 3 after 2to3 conversion.
@mgiuca-google
Copy link
Contributor Author

OK thanks for the explanation. What I don't get is if Python 2 has relative imports by default, why does it work? (It only stopped working in Python 3.)

Oh well, I've added the absolute import so we'll see what Travis says. Also rebased.

pelson added a commit that referenced this pull request Mar 20, 2013
Work around missing subprocess members on Google App Engine
@pelson pelson merged commit 8ec9555 into matplotlib:master Mar 20, 2013
@mgiuca-google mgiuca-google deleted the subprocess-google-app-engine branch March 21, 2013 03:21
@daradib
Copy link
Contributor

daradib commented Mar 3, 2014

Looks like 70f2296 added the line CalledProcessError = subprocess.CalledProcessError to compat/subprocess.py which causes an exception to raised since the attribute doesn't exist.

@mdboom
Copy link
Member

mdboom commented Mar 3, 2014

@daradib: If you're a Google App Engine user, would you mind writing up a PR that works around the issue?

@daradib
Copy link
Contributor

daradib commented Mar 3, 2014

@mdboom Ok, sure. I submitted #2860 to the maintenance branch because it's a tiny bugfix. If I should set it to master, let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants